@yoavGrs reviewed 4 files and all commit messages, and made 7 comments.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on ArielElp).

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 24 at r2 (raw file):

#[derive(Debug, Error, PartialEq, Eq)]
pub enum ProofVerificationError {

Usually, in tests, we panic on invalid cases. WDYT?

Code quote:

enum ProofVerificationError

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 57 at r2 (raw file):

) -> Result<ProofIndexMaps, ProofVerificationError> {
    let mut hash_by_index = HashMap::from([(NodeIndex::ROOT, root_hash)]);
    let mut parent_by_index = HashMap::new();

If you want:

impl ProofIndexMaps {
    fn insert(index, hash, parent_hash
}

Suggestion, non-blocking.

Suggestion:

    let mut index_maps = ProofIndexMaps{
        hash_by_index: HashMap::from([(NodeIndex::ROOT, root_hash)]),
        parent_by_index: HashMap::new(),
    }

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 115 at r2 (raw file):

}

/// Verifies that `preimages` contains a path from the root to each of the leaves in `leaf_indices`.

The nodes in the maps are validated,

Suggestion:

`maps`

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 121 at r2 (raw file):

    root_hash: HashOutput,
    leaf_indices: &[NodeIndex],
    leaf_hashes: &HashMap<NodeIndex, HashOutput>,

Please extract the validation that this hash-map (:smile:) is contained in the hash-by-index map, and remove this arg.

Code quote:

    leaf_hashes: &HashMap<NodeIndex, HashOutput>,

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 155 at r2 (raw file):

                .ok_or(ProofVerificationError::MissingNode { index: current_index })?;
            if !is_valid_child(current_index, parent_index, preimage) {
                return Err(ProofVerificationError::MissingNode { index: current_index });

You are validating it in build_proof_index_maps.

Code quote:

            if !is_valid_child(current_index, parent_index, preimage) {
                return Err(ProofVerificationError::MissingNode { index: current_index });

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 169 at r2 (raw file):

        }),
        None => Err(ProofVerificationError::MissingNode { index: NodeIndex::ROOT }),
    }

1. It's not part of the loop.
2. You inserted the root value in build_proof_index_maps. You don't need to check it.

Code quote:

    match maps.hash_by_index.get(&NodeIndex::ROOT) {
        Some(proof_value) if *proof_value == root_hash => Ok(()),
        Some(proof_value) => Err(ProofVerificationError::HashMismatch {
            index: NodeIndex::ROOT,
            proof_value: *proof_value,
            actual: root_hash,
        }),
        None => Err(ProofVerificationError::MissingNode { index: NodeIndex::ROOT }),
    }

@yoavGrs reviewed 4 files and all commit messages, and made 7 comments.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on ArielElp).

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 24 at r2 (raw file):

#[derive(Debug, Error, PartialEq, Eq)]
pub enum ProofVerificationError {

Usually, in tests, we panic on invalid cases. WDYT?

Code quote:

enum ProofVerificationError

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 57 at r2 (raw file):

) -> Result<ProofIndexMaps, ProofVerificationError> {
    let mut hash_by_index = HashMap::from([(NodeIndex::ROOT, root_hash)]);
    let mut parent_by_index = HashMap::new();

If you want:

impl ProofIndexMaps {
    fn insert(index, hash, parent_hash
}

Suggestion, non-blocking.

Suggestion:

    let mut index_maps = ProofIndexMaps{
        hash_by_index: HashMap::from([(NodeIndex::ROOT, root_hash)]),
        parent_by_index: HashMap::new(),
    }

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 115 at r2 (raw file):

}

/// Verifies that `preimages` contains a path from the root to each of the leaves in `leaf_indices`.

The nodes in the maps are validated,

Suggestion:

`maps`

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 121 at r2 (raw file):

    root_hash: HashOutput,
    leaf_indices: &[NodeIndex],
    leaf_hashes: &HashMap<NodeIndex, HashOutput>,

Please extract the validation that this hash-map (:smile:) is contained in the hash-by-index map, and remove this arg.

Code quote:

    leaf_hashes: &HashMap<NodeIndex, HashOutput>,

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 155 at r2 (raw file):

                .ok_or(ProofVerificationError::MissingNode { index: current_index })?;
            if !is_valid_child(current_index, parent_index, preimage) {
                return Err(ProofVerificationError::MissingNode { index: current_index });

You are validating it in build_proof_index_maps.

Code quote:

            if !is_valid_child(current_index, parent_index, preimage) {
                return Err(ProofVerificationError::MissingNode { index: current_index });

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 169 at r2 (raw file):

        }),
        None => Err(ProofVerificationError::MissingNode { index: NodeIndex::ROOT }),
    }

1. It's not part of the loop.
2. You inserted the root value in build_proof_index_maps. You don't need to check it.

Code quote:

    match maps.hash_by_index.get(&NodeIndex::ROOT) {
        Some(proof_value) if *proof_value == root_hash => Ok(()),
        Some(proof_value) => Err(ProofVerificationError::HashMismatch {
            index: NodeIndex::ROOT,
            proof_value: *proof_value,
            actual: root_hash,
        }),
        None => Err(ProofVerificationError::MissingNode { index: NodeIndex::ROOT }),
    }

@yoavGrs reviewed 4 files and all commit messages, and made 7 comments.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on ArielElp).

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 24 at r2 (raw file):

#[derive(Debug, Error, PartialEq, Eq)]
pub enum ProofVerificationError {

Usually, in tests, we panic on invalid cases. WDYT?

Code quote:

enum ProofVerificationError

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 57 at r2 (raw file):

) -> Result<ProofIndexMaps, ProofVerificationError> {
    let mut hash_by_index = HashMap::from([(NodeIndex::ROOT, root_hash)]);
    let mut parent_by_index = HashMap::new();

If you want:

impl ProofIndexMaps {
    fn insert(index, hash, parent_hash
}

Suggestion, non-blocking.

Suggestion:

    let mut index_maps = ProofIndexMaps{
        hash_by_index: HashMap::from([(NodeIndex::ROOT, root_hash)]),
        parent_by_index: HashMap::new(),
    }

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 115 at r2 (raw file):

}

/// Verifies that `preimages` contains a path from the root to each of the leaves in `leaf_indices`.

The nodes in the maps are validated,

Suggestion:

`maps`

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 121 at r2 (raw file):

    root_hash: HashOutput,
    leaf_indices: &[NodeIndex],
    leaf_hashes: &HashMap<NodeIndex, HashOutput>,

Please extract the validation that this hash-map (:smile:) is contained in the hash-by-index map, and remove this arg.

Code quote:

    leaf_hashes: &HashMap<NodeIndex, HashOutput>,

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 155 at r2 (raw file):

                .ok_or(ProofVerificationError::MissingNode { index: current_index })?;
            if !is_valid_child(current_index, parent_index, preimage) {
                return Err(ProofVerificationError::MissingNode { index: current_index });

You are validating it in build_proof_index_maps.

Code quote:

            if !is_valid_child(current_index, parent_index, preimage) {
                return Err(ProofVerificationError::MissingNode { index: current_index });

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 169 at r2 (raw file):

        }),
        None => Err(ProofVerificationError::MissingNode { index: NodeIndex::ROOT }),
    }

1. It's not part of the loop.
2. You inserted the root value in build_proof_index_maps. You don't need to check it.

Code quote:

    match maps.hash_by_index.get(&NodeIndex::ROOT) {
        Some(proof_value) if *proof_value == root_hash => Ok(()),
        Some(proof_value) => Err(ProofVerificationError::HashMismatch {
            index: NodeIndex::ROOT,
            proof_value: *proof_value,
            actual: root_hash,
        }),
        None => Err(ProofVerificationError::MissingNode { index: NodeIndex::ROOT }),
    }

@ArielElp made 6 comments and resolved 3 discussions.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on yoavGrs).

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 24 at r2 (raw file):

I prefer to pinpoint what goes wrong in the proof. May also allow easier transition of this into prod code if we ever want it in the future.

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 57 at r2 (raw file):

Resolved given that there's only one map now

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 115 at r2 (raw file):

maps is just a byproduct that helps me write the traversal more conveniently, the source of truth is preimages (every step "claimed" by the maps is verified w.r.t preimages).

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 121 at r2 (raw file):

Done.

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 155 at r2 (raw file):

Right, removed.

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 169 at r2 (raw file):

Right...

I realize now after separating hash verification from the traversal, that there is no need for a bottom-up traversal at all. It suffices to:

1. Build an index-->hash map from preimages, setting the children indices along the way (i.e. proof doesn't control them), and verify hashes
2. Verify that the expected leaf hashes are in the map from 1 (leaves get to 1 from their parent's preimage)

So I can trim much more, no need for the index-->parent mapping

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit c96290d. Configure here.}

@yoavGrs reviewed 2 files and all commit messages, made 5 comments, and resolved 4 discussions.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on ArielElp).

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 32 at r4 (raw file):

    preimages: &PreimageMap,
    leaf_indices: &[NodeIndex],
    leaf_hashes: &HashMap<NodeIndex, HashOutput>,

It seems to be enough.

Suggestion:

    leaf_hashes: &HashMap<NodeIndex, HashOutput>,

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 36 at r4 (raw file):

    if leaf_indices.is_empty() {
        return Ok(());
    }

Can you remove it?

Code quote:

    if leaf_indices.is_empty() {
        return Ok(());
    }

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 70 at r4 (raw file):

    root_hash: HashOutput,
    preimages: &PreimageMap,
) -> Result<HashMap<NodeIndex, HashOutput>, ProofVerificationError> {

Doc the returned object.

Code quote:

HashMap<NodeIndex, HashOutput>

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 78 at r4 (raw file):

        let Some(preimage) = preimages.get(&hash) else {
            continue;

Please add a comment about the state at that point.

Code quote:

continue;

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 78 at r4 (raw file):

        let Some(preimage) = preimages.get(&hash) else {
            continue;

Can you collect only these nodes into hash_by_index? So you return leaves + unmodified subtree roots - the nodes are not in preimages?

Code quote:

continue;

@ArielElp made 5 comments and resolved 1 discussion.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on yoavGrs).

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 32 at r4 (raw file):

Done.

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 36 at r4 (raw file):

Yes, this is not important for soundness or anything, but it makes sense IMO. No need to check preimages if you don't care about any leaf.

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 70 at r4 (raw file):

There is already doc over this function that starts with:

/// Builds an `index -> hash` map by expanding a Patricia proof from the root.

I think the mapping itself is clear from the types (index --> hash).

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 78 at r4 (raw file):

Done.

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 78 at r4 (raw file):

Why only these nodes? More intuitive to just build the entire mapping IMO

@yoavGrs reviewed 1 file and all commit messages, made 2 comments, and resolved 5 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on ArielElp).

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 78 at r4 (raw file):

Because you already have the preimages as input. It makes sense to return the new nodes.
I suggest using a queue of hashes (instead of indices), and appending to ' hash_by_index' only the nodes that enter the continue step.

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 74 at r5 (raw file):

        let Some(preimage) = preimages.get(&hash) else {
            // We reach a child-node of a node in `preimages` that is supposedly not required in
            // the proof (e.g. sibling of a node with no requested leaves in its subtree).

Or a leaf?

Code quote:

sibling of a node with no requested leaves in its subtree

@dorimedini-starkware reviewed 4 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on ArielElp).

crates/starknet_patricia/src/patricia_merkle_tree.rs line 5 at r5 (raw file):

pub mod node_data;
pub mod original_skeleton_tree;
#[cfg(feature = "testing")]

non-blocking, but these usually come together; if there are no tests in the patricia crate that will use it then it's ok to keep it as is

Suggestion:

#[cfg(any(test, feature = "testing"))]

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 74 at r5 (raw file):

        let Some(preimage) = preimages.get(&hash) else {
            // We reach a child-node of a node in `preimages` that is supposedly not required in
            // the proof (e.g. sibling of a node with no requested leaves in its subtree).

I'm confused, isn't this what you mean?

Suggestion:

            // We reach a child-node of a node in `preimages` that is supposedly not required in
            // the proof (e.g. child of a sibling of a node with requested leaves in its subtree).

@ArielElp made 3 comments.
Reviewable status: 3 of 4 files reviewed, 4 unresolved discussions (waiting on dorimedini-starkware and yoavGrs).

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 78 at r4 (raw file):

It feels somewhat random to return dead ends of preimages rather than the validated nodes in a nice tree structure.

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 74 at r5 (raw file):

Done.

crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 74 at r5 (raw file):

No, but maybe I need to rephrase to make it clear. I mean "dead ends" referrenced in preimages, which if honest, are unmodified children of modified nodes.

@dorimedini-starkware reviewed 1 file and all commit messages, and resolved 1 discussion.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on ArielElp and yoavGrs).

@ArielElp made 1 comment and resolved 1 discussion.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on yoavGrs).

crates/starknet_patricia/src/patricia_merkle_tree.rs line 5 at r5 (raw file):

At the moment it's only used in apollo_committer

@yoavGrs reviewed 1 file and resolved 2 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on ArielElp).

@yoavGrs made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on ArielElp).

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on ArielElp).